Allow rules without codes - #28049
Conversation
ntBre
added
preview
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 97.79%. The percentage of expected errors that received a diagnostic held steady at 94.33%. The number of fully passing files held steady at 112/136. |
Memory usage reportMemory usage unchanged ✅ |
| (Flake8UsePathlib, "124") => rules::flake8_use_pathlib::violations::PyPath, | ||
| (Flake8UsePathlib, "201") => rules::flake8_use_pathlib::rules::PathConstructorCurrentDirectory, | ||
| (Flake8UsePathlib, "202") => rules::flake8_use_pathlib::rules::OsPathGetsize, | ||
| (Flake8UsePathlib, "202") => rules::flake8_use_pathlib::rules::OsPathGetsize, |
There was a problem hiding this comment.
Oops. Unrelated but noticed while I was here.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| pytest-fixture-autouse | 740 | 740 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
b36a3c4 to
8e75e0c
Compare
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you.
I think I'd prefer if we used rule.identifier in fewer places.
- Let's move existing tests. While a little more work, it's not that involved and it simplifies the tests, they can simply call name. This also makes the test future prove for when we remove or deprecate codes. It also expresses that names are the preferred default now (I can't wait for when I don't need to remember both)
- Let's review the uses of
identifierinSupport lint rules without legacy codes. Are there places where we can just usenamebecause it isn't user facing? Are some of the uses user facing and need preview gating...
| pub fn identifier(&self) -> LintIdentifier { | ||
| match self.noqa_code() { | ||
| Some(code) => LintIdentifier::Code(code), | ||
| None => LintIdentifier::Name(self.name()), | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the reason that we continue preferring the code over its name? Why can't we always use the name as identifier? Do we need to preview gate this?
Overall, the name identifier seems a bit confusing to me. A rule's unique identifier now and going forward is always its name. This seems to be mainly a compatibility layer, but it can't be for anything user facing, because it would otherwise have to be preview gated?
There was a problem hiding this comment.
Codex is telling me that this breaks the ruff server.
With preview enabled and
output-prefer-rule-codes = true, the server requests Noqa-style suppression edits, so this branch drops the edit forpytest-fixture-autouse. I reproduced that its “Disable for this line” action disappears; setting the option to false restores it. Fall back to a name-basedruff: ignoreedit for codeless diagnostics in the server, regardless of the output preference.
There was a problem hiding this comment.
My Codex told me this as well, but I don't think this is actually a problem. This requires:
- preview enabled
- output-prefer-rule-codes = true
- activating the preview, category-only rule
pytest-fixture-autousethat is also quite pedantic - trying to quick-fix the rule in an editor
and the only "breakage" is that we don't offer the ruff: ignore quickfix when you've expressed a preference for codes. My codex also tried to fix this and it seemed more trouble than it was worth to allow changing the suppression style per diagnostic (it's currently selected for the whole batch of diagnostics).
I also wanted to fix it initially but was put off at least by Codex's initial approach, and it's quite a niche use case anyway. I can take a closer look if you want, though.
| let mut rule_names: Vec<_> = incompatible_rules | ||
| .into_iter() | ||
| .map(|rule| format!("`{}`", rule.noqa_code())) | ||
| .map(|rule| format!("`{}`", rule.identifier())) |
There was a problem hiding this comment.
This use seems user visible. Do we need to preview gate it?
There was a problem hiding this comment.
This is already preview-gated by virtue of the fact that the name-only rules are going to be in preview. There's no other option for rules without codes than to show the name, so I don't think there's any additional level of preview-gating we can apply. That's why identifier prefers codes when they're available, to preserve compatibility.
There was a problem hiding this comment.
Actually, we could just unwrap here for now. This is only used to warn for two rules that definitely have noqa codes.
Alternatively, are warnings part of our stable interface? It doesn't seem totally crazy to me just to switch this always to use names.
This also applies to this warning in the server:
ruff/crates/ruff_server/src/server/api/requests/hover.rs
Lines 147 to 148 in 26f4f73
We're down to only 3 uses of identifier including these two warnings.
There was a problem hiding this comment.
Curious what you'll think about this, but I went ahead and unwrapped noqa_code here, used name_and_code in the server warning, and inlined identifier into RUF105, which allowed me to delete identifier completely.
There was a problem hiding this comment.
Alternatively, are warnings part of our stable interface? It doesn't seem totally crazy to me just to switch this always to use names.
They're part of what I consider Ruff's user interface. Which is why I'd expect it to follow my configuration. But I admit, it's probably not worth bothering too much about it, but we should use codes or names based on the user's configuration if we can.
| let mut rule_names: Vec<_> = incompatible_rules | ||
| .into_iter() | ||
| .map(|rule| format!("`{}`", rule.noqa_code())) | ||
| .map(|rule| format!("`{}`", rule.identifier())) |
There was a problem hiding this comment.
Isn't this user facing? I think we need to use names here in preview
| .inner | ||
| .iter() | ||
| .sorted_by_key(|diagnostic| diagnostic.secondary_code()) | ||
| .sorted_by_key(|diagnostic| diagnostic.secondary_code_or_id()) |
There was a problem hiding this comment.
Why do we group by secondary_code_or_id and not by id and only store the secondary code if needed for display?
There was a problem hiding this comment.
Ah good catch, I didn't realize we were calling secondary_code again down below for the output. We can use name here too. I always find this whole fold pretty complicated to follow.
There was a problem hiding this comment.
Well, codex pointed out that using name here has a very small impact on the stable output because the second sort below this won't reorder ties with the same count. So "F401 previously appeared before F821 [when they had the same count]; now F821 appears first." I think this is probably fine?
There was a problem hiding this comment.
I'd have to take a closer look, but if it's user facing, I'd expect it to follow my setting. Again, I agree, that's a detail, so probably not worth spending much time on, but it degrades polish
There was a problem hiding this comment.
Maybe an example is helpful here. I definitely know what you mean about degrading polish, and I think I've made two such suggestions on this PR 🙁 But the difference here is something like this:
105 F403 undefined-local-with-import-star
2 E402 module-import-not-at-top-of-file
- 1 B007 unused-loop-control-variable
- 1 F821 undefined-name
+ 1 F821 undefined-name
+ 1 B007 unused-loop-control-variable
1 TID251 banned-api
1 TID253 banned-module-level-importsJust two rows flipping order in the --statistics output when they have the same count and their code-sort order differs from their name-sort order.
Of course, I can just put this back to secondary_code_or_id and restore the old sort order since there turned out to be a better answer to your very first question than I thought.
| for rule in rules { | ||
| let name = rule.name(); | ||
| let code = rule.noqa_code(); | ||
| let code = rule.identifier(); |
There was a problem hiding this comment.
Why not use name_and_code here (and above)?
There was a problem hiding this comment.
name_and_code formats exactly as name (code), but this formats them differently:
let _ = writeln!(output, "- [`{name}`](rules/{name}.md) (`{code}`)");This code is actually problematic for a different reason since it loops over linters, disregarding rules that don't belong to a linter group. I think for now I'll just use noqa_code().unwrap() in light of that.
We'll have to stabilize both the categories themselves and a rule without a code for this to be a real problem, and I'm planning to include a secondary, category-focused rule page in my migration guide PR anyway.
8f81cca to
2941dfc
Compare
|
Can you squash some of your commit? It's otherwise very hard to review this PR because I can't review all changes because of the million snapshot changes, but going commit by commit only gives me partial changes, that I then need to keep in mind |
9c5afd2 to
d8e40ef
Compare
|
I squashed this back into the original structure from the PR summary and updated the summary itself to reflect the test changes. There's now never a |
Summary -- This PR allows adding rules that only belong to categories and only have names rather than requiring a linter group and code. The first non-empty commit introduces a `Rule::identifier` shim and uses it to replace `Rule::noqa_code` calls mechanically in all the test snapshots. The second commit updates `FixTable` from a mapping of `SecondaryCode -> (name, fix_count)` to `DiagnosticId -> (Option<SecondaryCode>, fix_count)`. This doesn't really change its usage but involves minor diffs to a substantial number of lines. It also allows us to drop some of the special `hashbrown` handling and the `hashbrown` dependency in `ruff_linter`, which is nice. The third commit contains the bulk of the change, including the macro changes needed to support `map_codes` entries like that for the `pytest-fixture-autouse` rule in the fourth commit: ```rust () => rules::ruff::rules::PytestFixtureAutouse, ``` These changes are pretty mechanical overall, just handling cases where the linter or noqa code could be missing, which fortunately seemed pretty straightforward in every case. The fourth commit takes advantage of these changes to reinstate `RUF076` as our first purely `pedantic` rule. I'm happy to drop that if it's too controversial or opens too many questions about the previously-removed rule and its code, but I thought it was nice to have a motivating example. We moved this rule from `preview` to `removed` because it was too pedantic for the `RUF` category, even in preview, so it's exactly the kind of rule we can support now with the categories. Finally, the fifth commit updates the contributing docs to reflect that linters and codes are optional. This may need to be adjusted if we back out the `RUF076` change. Test Plan -- Existing tests, plus a few new ones for `pytest-fixture-autouse`
avoid path-length issues on windows
share initial count writing group by name in write_statistics use rule name in generate_docs simplify issue search query when code is unavailable expect noqa codes for linter-based rules remove Rule::identifier
d8e40ef to
1596c5a
Compare
| let mut rule_names: Vec<_> = incompatible_rules | ||
| .into_iter() | ||
| .map(|rule| format!("`{}`", rule.noqa_code())) | ||
| .map(|rule| format!("`{}`", rule.noqa_code().unwrap())) |
There was a problem hiding this comment.
This will be easy to miss when stabilizing rule names. Can we show name (code) instead?
There was a problem hiding this comment.
Sure, then I can drop the debug_assert too.
| || Rule::from_name(_rule) | ||
| .is_ok_and(|rule| matches!(rule.category(), Category::Testing)) |
There was a problem hiding this comment.
Why is matching by category not sufficient? Shouldn't it allow us to at least remove RUF9?
There was a problem hiding this comment.
The _rule binding here is a little misleading because this iterator includes both full rule names like RUF901 and also prefixes like RUF9. We want to filter both out. I was confused by this initially and asked Codex earlier.
I also tested this today to make sure, and everything looks fine when running cargo dev generate-all because the testing feature isn't active, but the generate_json_schema test fails.
Summary -- This PR allows adding rules that only belong to categories and only have names rather than requiring a linter group and code. The first non-empty commit uses `Rule::name` instead of `Rule::noqa_code` throughout the linter tests, causing a huge number of file renames without changes. The second commit updates `FixTable` from a mapping of `SecondaryCode -> (name, fix_count)` to `DiagnosticId -> (Option<SecondaryCode>, fix_count)`. This doesn't really change its usage but involves minor diffs to a substantial number of lines. It also allows us to drop some of the special `hashbrown` handling and the `hashbrown` dependency in `ruff_linter`, which is nice. The third commit contains the bulk of the change, including the macro changes needed to support `map_codes` entries like that for the `pytest-fixture-autouse` rule in the fourth commit: ```rust () => rules::ruff::rules::PytestFixtureAutouse, ``` These changes are pretty mechanical overall, just handling cases where the linter or noqa code could be missing, which fortunately seemed pretty straightforward in every case. The fourth commit takes advantage of these changes to reinstate `RUF076` as our first purely `pedantic` rule. I'm happy to drop that if it's too controversial or opens too many questions about the previously-removed rule and its code, but I thought it was nice to have a motivating example. We moved this rule from `preview` to `removed` because it was too pedantic for the `RUF` category, even in preview, so it's exactly the kind of rule we can support now with the categories. Finally, the fifth commit updates the contributing docs to reflect that linters and codes are optional. This may need to be adjusted if we back out the `RUF076` change. Test Plan -- Existing tests, plus a few new ones for `pytest-fixture-autouse`
2 participants
Summary
This PR allows adding rules that only belong to categories and only have names rather than requiring
a linter group and code.
The first non-empty commit uses
Rule::nameinstead ofRule::noqa_codethroughout the linter tests, causing a huge number of file renames without changes.The second commit updates
FixTablefrom a mapping ofSecondaryCode -> (name, fix_count)toDiagnosticId -> (Option<SecondaryCode>, fix_count). This doesn't really change its usage butinvolves minor diffs to a substantial number of lines. It also allows us to drop some of the special
hashbrownhandling and thehashbrowndependency inruff_linter, which is nice.The third commit contains the bulk of the change, including the macro changes needed to support
map_codesentries like that for thepytest-fixture-autouserule in the fourth commit:These changes are pretty mechanical overall, just handling cases where the linter or noqa code could
be missing, which fortunately seemed pretty straightforward in every case.
The fourth commit takes advantage of these changes to reinstate
RUF076as our first purelypedanticrule. I'm happy to drop that if it's too controversial or opens too many questions aboutthe previously-removed rule and its code, but I thought it was nice to have a motivating example. We
moved this rule from
previewtoremovedbecause it was too pedantic for theRUFcategory, evenin preview, so it's exactly the kind of rule we can support now with the categories.
Finally, the fifth commit updates the contributing docs to reflect that linters and codes are
optional. This may need to be adjusted if we back out the
RUF076change.Test Plan
Existing tests, plus a few new ones for
pytest-fixture-autouse